Friendly ping @intel/dpcpp-cfe-reviewers

Added a test to make sure that the values are indeed ignored: 0a72b7b

Test failure:
SYCL :: OptionalKernelFeatures/is_compatible/is_compatible_with_aspects.cpp
seems to be related to the patch.

Test failure: SYCL :: OptionalKernelFeatures/is_compatible/is_compatible_with_aspects.cpp seems to be related to the patch.

That's right, this patch changes the handling of incorrect sub-group attribute, such that it doesn't end up in the resulting binary image.
The way this test works is that it creates a kernel with a known, incorrect required sub group size (INT_MAX). Then expects that kernel to be incompatible with the device, by failing a check against supported sub-group sizes.

There are two ways of thinking about that issue:

• since we warn and ignore the incorrect value of sub-group size: https://github.com/intel/llvm/pull/11687/files#diff-2a5bdb2d9f07f8d77de51d5403d349c22978141b6de6bd87fc5e449f5ed95becR4027 this test is not applicable anymore,
• or we shouldn't ignore the incorrect values and only warn.

I'm not sure which is the correct way of handling those values.
Would you be able to advise @againull @elizabethandrews @smanna12 ?

Test failure: SYCL :: OptionalKernelFeatures/is_compatible/is_compatible_with_aspects.cpp seems to be related to the patch.

That's right, this patch changes the handling of incorrect sub-group attribute, such that it doesn't end up in the resulting binary image. The way this test works is that it creates a kernel with a known, incorrect required sub group size (INT_MAX). Then expects that kernel to be incompatible with the device, by failing a check against supported sub-group sizes.

There are two ways of thinking about that issue:

• since we warn and ignore the incorrect value of sub-group size: https://github.com/intel/llvm/pull/11687/files#diff-2a5bdb2d9f07f8d77de51d5403d349c22978141b6de6bd87fc5e449f5ed95becR4027 this test is not applicable anymore,
• or we shouldn't ignore the incorrect values and only warn.

I'm not sure which is the correct way of handling those values. Would you be able to advise @againull @elizabethandrews @smanna12 ?

I don't think it makes sense allowing an invalid subgroup size to pass through if we're emitting the warning. IMO we should either just allow the incorrect value to pass through without a warning and then have the backend deal with it however it is doing so currently, or we do what this PR does - i.e. emit warning and drop the attribute. @AlexeySachkov can you weigh in here? Is there a reason we are passing though invalid subgroup size?

Any preference on this @AlexeySachkov ?

Any preference on this @AlexeySachkov ?

@AlexeySachkov friendly ping on this one please.

Test failure: SYCL :: OptionalKernelFeatures/is_compatible/is_compatible_with_aspects.cpp seems to be related to the patch.

That's right, this patch changes the handling of incorrect sub-group attribute, such that it doesn't end up in the resulting binary image. The way this test works is that it creates a kernel with a known, incorrect required sub group size (INT_MAX). Then expects that kernel to be incompatible with the device, by failing a check against supported sub-group sizes.
There are two ways of thinking about that issue:

• since we warn and ignore the incorrect value of sub-group size: https://github.com/intel/llvm/pull/11687/files#diff-2a5bdb2d9f07f8d77de51d5403d349c22978141b6de6bd87fc5e449f5ed95becR4027 this test is not applicable anymore,
• or we shouldn't ignore the incorrect values and only warn.

I'm not sure which is the correct way of handling those values. Would you be able to advise @againull @elizabethandrews @smanna12 ?

I don't think it makes sense allowing an invalid subgroup size to pass through if we're emitting the warning. IMO we should either just allow the incorrect value to pass through without a warning and then have the backend deal with it however it is doing so currently, or we do what this PR does - i.e. emit warning and drop the attribute. @AlexeySachkov can you weigh in here? Is there a reason we are passing though invalid subgroup size?

From SYCL spec point of view, there are no compile-time known incorrect sub-group sizes - what we have for AMD and CUDA are implementation details of those backends.

Explicitly requesting sub-group size of a kernel has limited portability by design, but considering how fundamental and narrow the selection of possible sub-group sizes for AMD/CUDA, it might make sense to promote that knowledge into the compiler in form of the suggested warning to improve user experience.

I think that it may actually make sense to pass the invalid sizes through even if we diagnosed them with a warning: this way runtime handling of the attribute for AMD/CUDA backend would be uniform with other backends and users will get more consistent experience. At the same time the warning will let users know in advance that their application won't work and it requires some code changes.

The only concern about the warning I have is that it may produce false alarms: what if there is a kernel which is never submitted to a AMD/CUDA device? The warning will still be there and it may be annoying for someone who uses -Werror. Once we get proper support for optional kernel features for AOT targets it may be even easier to discover that

Test failure: SYCL :: OptionalKernelFeatures/is_compatible/is_compatible_with_aspects.cpp seems to be related to the patch.

That's right, this patch changes the handling of incorrect sub-group attribute, such that it doesn't end up in the resulting binary image. The way this test works is that it creates a kernel with a known, incorrect required sub group size (INT_MAX). Then expects that kernel to be incompatible with the device, by failing a check against supported sub-group sizes.
There are two ways of thinking about that issue:

• since we warn and ignore the incorrect value of sub-group size: https://github.com/intel/llvm/pull/11687/files#diff-2a5bdb2d9f07f8d77de51d5403d349c22978141b6de6bd87fc5e449f5ed95becR4027 this test is not applicable anymore,
• or we shouldn't ignore the incorrect values and only warn.

I'm not sure which is the correct way of handling those values. Would you be able to advise @againull @elizabethandrews @smanna12 ?

I don't think it makes sense allowing an invalid subgroup size to pass through if we're emitting the warning. IMO we should either just allow the incorrect value to pass through without a warning and then have the backend deal with it however it is doing so currently, or we do what this PR does - i.e. emit warning and drop the attribute. @AlexeySachkov can you weigh in here? Is there a reason we are passing though invalid subgroup size?

From SYCL spec point of view, there are no compile-time known incorrect sub-group sizes - what we have for AMD and CUDA are implementation details of those backends.

Explicitly requesting sub-group size of a kernel has limited portability by design, but considering how fundamental and narrow the selection of possible sub-group sizes for AMD/CUDA, it might make sense to promote that knowledge into the compiler in form of the suggested warning to improve user experience.

I think that it may actually make sense to pass the invalid sizes through even if we diagnosed them with a warning: this way runtime handling of the attribute for AMD/CUDA backend would be uniform with other backends and users will get more consistent experience. At the same time the warning will let users know in advance that their application won't work and it requires some code changes.

Following you suggestion, I've let the invalid value pass through. I've removed a test, since no values are being ignored.

The only concern about the warning I have is that it may produce false alarms: what if there is a kernel which is never submitted to a AMD/CUDA device? The warning will still be there and it may be annoying for someone who uses -Werror. Once we get proper support for optional kernel features for AOT targets it may be even easier to discover that

I feel that it's an edge case, that is outweighed by the benefit of informing the users about the incorrect size, and perhaps we could ignore it for now. If that ever becomes an issue, it would be simple enough to provide a mechanism to silence this particular warning?

The only concern about the warning I have is that it may produce false alarms: what if there is a kernel which is never submitted to a AMD/CUDA device? The warning will still be there and it may be annoying for someone who uses -Werror. Once we get proper support for optional kernel features for AOT targets it may be even easier to discover that

I feel that it's an edge case, that is outweighed by the benefit of informing the users about the incorrect size, and perhaps we could ignore it for now. If that ever becomes an issue, it would be simple enough to provide a mechanism to silence this particular warning?

I think that we may encounter that sooner than later, but I do agree that benefit of the warning probably outweighs the -Werror concern. I'm fine with the direction of this PR, but we should keep in mind that at some point we will likely need a knob to disable that particular warning.

I feel that it's an edge case, that is outweighed by the benefit of informing the users about the incorrect size, and perhaps we could ignore it for now. If that ever becomes an issue, it would be simple enough to provide a mechanism to silence this particular warning?

As a person compiling with -Werror and for multiple sub-group sizes (toy example), I mostly agree there; being able to suppress the warning is enough.

It looks like currently the required error suppression would be too broad. Originally (#6183), -Wno-cuda-compat was enough, which does not suppress too much relevant warnings. Now, -Wno-unknown-attributes would be needed, which can hide true positives.